Skip to content

Comments

Warm start in Eigen interface#95

Merged
darnstrom merged 10 commits intomasterfrom
eigen-warmstart
Dec 7, 2025
Merged

Warm start in Eigen interface#95
darnstrom merged 10 commits intomasterfrom
eigen-warmstart

Conversation

@darnstrom
Copy link
Owner

No description provided.

Comment on lines +225 to +226
if(sense_ptr == nullptr)
sense_ptr = work_.sense; // Directly work with sense of workspace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this should always be the case because sense is set to empty, when we call solve with inputs.
Does it mean that it should be a member of the class instead?
Or can we just always use work_.sense and delete sense?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should always be able to use work_.sense. Moreover, sense might be non empty if the update(...) + solve() method is used rather than just solve(...).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is that in this case we can just assign sense to work_.sense and remove sense_ptr. The latter seems redundant.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this would not work in the case when the user calls DAQP::update with their own sense! I agree, however, that it would work if one would only call the solve with A,bu and bl, since sense is always set to null in that call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we could do work_.sense = sense.data() similarly to what we do right now in the update.
Couldn't that work?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

work_.sense points to memory allocated internally by DAQP, so to avoid memory leaks it should not be altered.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see and it would then try to free stuff owned by the eigen variable.
Thanks for clarifying.

Comment on lines 248 to 250
if(true){

}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIP or leftover?

Comment on lines +271 to +278
void DAQP::set_warm_start() {
warm_start_ = true;
}

void DAQP::set_cold_start() {
warm_start_ = false;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the behavior depends on sense, could it make sense (pun intended :D) to reset that if one wants to cold start, remove the flag and basically warm start as default?

To me, the user is probably interested in using the solver class because there are more related problems to solve. Therefore, warm starting should be the preferred option.

What do you think?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the most recent change it should make sense to warm start by default (although, as is always the case with warm starting, it might exacerbate the worst-case solution time if the new problem is not so similar to the previously solved)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, if we remove warm_start_ and in set_cold_start what we do is work_.sense = nullptr is this enough?

Copy link
Owner Author

@darnstrom darnstrom Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that could work I think! I am still not fully convinced that warm starting should be the default though. It depends on if we want to improve the average or the worst-case behavior.

@darnstrom darnstrom merged commit a0a7877 into master Dec 7, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants